plugwise_usb/nodes/circle.py (4)

549-551: Typo in comment

“store am empty record instead” → “store an empty record instead”.

---

668-672: Ensure deterministic cache serialization order

The comment says “logs is already sorted in reverse”, but dict ordering depends on insertion order. Unless EnergyCounters.get_pulse_logs guarantees reverse insertion, cache order may become unstable. Consider explicit sorting and iterating items directly:

-        # logs is already sorted in reverse
-        for address, record in logs.items():
-            for slot in record:
-                log = record[slot]
+        # Ensure deterministic reverse order
+        for address, record in sorted(logs.items(), reverse=True):
+            for slot, log in sorted(record.items(), reverse=True):

This trades minimal CPU for stable cache diffs and simpler troubleshooting.

---

710-711: Avoid trailing delimiter when cache is empty

When cached_logs == "", the assignment produces "new|", leaving a trailing pipe. While harmless on load, it’s easy to avoid:

-                self._set_cache(
-                    CACHE_ENERGY_COLLECTION, log_cache_record + "|" + cached_logs
-                )
+                sep = "" if cached_logs == "" else "|"
+                self._set_cache(
+                    CACHE_ENERGY_COLLECTION, log_cache_record + sep + cached_logs
+                )

---

456-466: Docstring vs. implementation: off-by-one in number of addresses

Docstring says “last 10 log addresses,” but total_addresses is 11, including current + 10 previous. Either update the docstring to “last 11 (current + 10 previous) log addresses” or set total_addresses = 10 to match the text.

Also applies to: 468-476

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:615-627
Timestamp: 2025-06-20T07:58:41.534Z
Learning: In the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `None` means the parameter has not been initialized yet, not that all logs are present. When `log_addresses_missing` is `None`, returning `False` from cache loading methods is correct because the system is not properly initialized and further execution would be useless.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.163Z
Learning: In the plugwise_usb codebase, the `_load_defaults` method in node classes (scan.py, switch.py, sense.py) is only called once during node initialization, not during runtime. Therefore, flags like `_sed_node_info_update_task_scheduled` don't need to be reset at the start of `_load_defaults` as there's no risk of stale state from previous loads.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:615-627
Timestamp: 2025-06-20T07:58:41.534Z
Learning: In the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `None` means the parameter has not been initialized yet, not that all logs are present. When `log_addresses_missing` is `None`, returning `False` from cache loading methods is correct because the system is not properly initialized and further execution would be useless.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.163Z
Learning: In the plugwise_usb codebase, the `_load_defaults` method in node classes (scan.py, switch.py, sense.py) is only called once during node initialization, not during runtime. Therefore, flags like `_sed_node_info_update_task_scheduled` don't need to be reset at the start of `_load_defaults` as there's no risk of stale state from previous loads.

plugwise_usb/nodes/circle.py (1)

579-598: Previous review already noted that logging every outdated slot at WARNING may spam logs during catch-up; consider downgrading to DEBUG/INFO.

plugwise_usb/nodes/circle.py (3)

468-475: Rephrase debug message for clarity

“Energy None-data of outdated data …” is hard to parse; consider:

- _LOGGER.debug(
-     "Energy None-data of outdated data collected from log address %s, stopping collection",
-     log_address,
- )
+ _LOGGER.debug(
+     "All slots at log address %s are empty or outdated – stopping initial collection",
+     log_address,
+ )

---

511-578: Clarify return semantics in docstring

energy_log_update now returns True only when at least one recent, non-empty record was stored.
Update the docstring accordingly to avoid confusion for future callers.

- """Request energy log statistics from node. Returns true if successful."""
+ """Request energy logs and return True only when at least one recent, non-empty
+    record was stored; otherwise return False."""

---

667-676: Don’t rely on dict insertion order for cache serialization

logs is assumed “already sorted”, but dict key order is an implementation detail of EnergyCounters. For deterministic cache output:

- for address, record in logs.items():
+ for address in sorted(logs.keys(), reverse=True):
+     record = logs[address]

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:615-627
Timestamp: 2025-06-20T07:58:41.534Z
Learning: In the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `None` means the parameter has not been initialized yet, not that all logs are present. When `log_addresses_missing` is `None`, returning `False` from cache loading methods is correct because the system is not properly initialized and further execution would be useless.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.163Z
Learning: In the plugwise_usb codebase, the `_load_defaults` method in node classes (scan.py, switch.py, sense.py) is only called once during node initialization, not during runtime. Therefore, flags like `_sed_node_info_update_task_scheduled` don't need to be reset at the start of `_load_defaults` as there's no risk of stale state from previous loads.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:615-627
Timestamp: 2025-06-20T07:58:41.534Z
Learning: In the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `None` means the parameter has not been initialized yet, not that all logs are present. When `log_addresses_missing` is `None`, returning `False` from cache loading methods is correct because the system is not properly initialized and further execution would be useless.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.163Z
Learning: In the plugwise_usb codebase, the `_load_defaults` method in node classes (scan.py, switch.py, sense.py) is only called once during node initialization, not during runtime. Therefore, flags like `_sed_node_info_update_task_scheduled` don't need to be reset at the start of `_load_defaults` as there's no risk of stale state from previous loads.

plugwise_usb/nodes/circle.py (5)

368-371: Naming may mislead: _last_energy_log_requested now stores a success boolean

Since energy_log_update returns “stored any recent record”, assigning it to _last_energy_log_requested conflates “requested” with “had data”. Consider renaming to reflect semantics (e.g., _initial_energy_logs_collected) or add a clarifying comment.

-            self._last_energy_log_requested = await self.energy_log_update(
+            # Tracks whether this initial request yielded at least one recent, non-empty record
+            self._last_energy_log_requested = await self.energy_log_update(
                 self._current_log_address
             )

---

467-475: Break-on-empty/outdated now works; check wording vs count and log style

• Behavior: good. The loop now exits when only empty/outdated slots are returned.
• Minor: the debug above says “last 10 log addresses” but total_addresses is 11; verify intended count.
• Minor: consider replacing the en dash with a hyphen in logs for consistency across environments.

Would you like to adjust total_addresses or the log message for consistency?

---

541-549: Typo in comment: “am empty” → “an empty”

Small grammar fix.

-                # Don't store an old log-record, store am empty record instead
+                # Don't store an old log record, store an empty record instead

---

569-576: Avoid redundant cache saves during batched retrievals

energy_log_update saves the cache for each address. In get_missing_energy_logs you already save once after awaiting all tasks. To reduce churn and potential interleaving, make saving optional and disable it for batch calls.

-    async def energy_log_update(self, address: int | None) -> bool:
+    async def energy_log_update(self, address: int | None, *, save_cache: bool = True) -> bool:
@@
-        if any_record_stored:
+        if any_record_stored and save_cache:
             _LOGGER.debug(
                 "Saving energy record update to cache for %s", self._mac_in_str
             )
             await self.save_cache()

Then call with save_cache=False in batched contexts:

# get_missing_energy_logs
tasks = [create_task(self.energy_log_update(address, save_cache=False)) for address in missing_addresses]
# _get_initial_energy_logs
result = await self.energy_log_update(log_address, save_cache=False)

---

578-597: 24h boundary bug fixed; consider lowering log level to avoid noise

Using a direct seconds comparison removes the floor-division bug. Consider downgrading the warning to info/debug during catch-up to avoid log spam when many outdated entries are encountered.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.

plugwise_usb/nodes/circle.py (1)

551-558: Critical: Return value still depends on cache being enabled; decouple any_record_stored.
This repeats a previously flagged issue: _energy_log_record_update_state returns False when cache is disabled, so energy_log_update will incorrectly return False even when a valid, recent record was added to in-memory counters. This breaks initial collection and other callers that rely on True meaning “we updated something.”

Apply this diff to set any_record_stored independent of cache writes:

-            if await self._energy_log_record_update_state(
+            # Store the record; treat it as "stored" regardless of cache availability
+            await self._energy_log_record_update_state(
                 response.log_address,
                 _slot,
                 log_timestamp.replace(tzinfo=UTC),
                 log_pulses,
                 import_only=True,
-            ):
-                any_record_stored = True
+            )
+            any_record_stored = True

plugwise_usb/nodes/circle.py (2)

542-549: Nit: fix comment typo and clarity (“am” -> “an”).
Minor, but it reads better and avoids confusion.

-                # Don't store an old log-record, store am empty record instead
+                # Don't store an old log-record, store an empty record instead

---

681-685: Ensure deterministic slot ordering when serializing cache.
Relying on insertion order can be fragile; explicitly sort slots in reverse to match the intended “already reversed” invariant.

-        # logs is already sorted in reverse
-        for address, record in logs.items():
-            for slot in record:
+        # logs is already sorted in reverse on addresses; sort slots explicitly
+        for address, record in logs.items():
+            for slot in sorted(record.keys(), reverse=True):
                 log = record[slot]

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.

plugwise_usb/nodes/circle.py (1)

599-654: Cache loading logic needs consistency with MAX_LOG_HOURS

The cache loading and pruning logic uses DAY_IN_HOURS for the skip_before threshold. For consistency with the PR objective and the rest of the codebase, this should use MAX_LOG_HOURS.

        # Sort and prune the records loaded from cache
        sorted_logs: dict[int, dict[int, tuple[int, datetime]]] = {}
-       skip_before = datetime.now(tz=UTC) - timedelta(hours=DAY_IN_HOURS)
+       skip_before = datetime.now(tz=UTC) - timedelta(hours=MAX_LOG_HOURS)
        sorted_addresses = sorted(restored_logs.keys(), reverse=True)

plugwise_usb/nodes/circle.py (1)

578-597: Recency check should use MAX_LOG_HOURS, not DAY_IN_HOURS

Per the PR objective of handling logs outside MAX_LOG_HOURS, the recency check should use MAX_LOG_HOURS instead of the hardcoded DAY_IN_HOURS (24h). This ensures consistency with the pruning logic used elsewhere in the codebase.

    def _check_timestamp_is_recent(
        self, address: int, slot: int, timestamp: datetime
    ) -> bool:
        """Check if the timestamp of the received log-record is recent.

-       A timestamp from within the last 24 hours is considered recent.
+       A timestamp newer than MAX_LOG_HOURS is considered recent.
        """
        age_seconds = (
            datetime.now(tz=UTC) - timestamp.replace(tzinfo=UTC)
        ).total_seconds()
-       if age_seconds > DAY_IN_HOURS * 3600:
+       if age_seconds > MAX_LOG_HOURS * 3600:
            _LOGGER.warning(
                "EnergyLog from Node %s | address %s | slot %s | timestamp %s is outdated, ignoring...",
                self._mac_in_str,
                address,
                slot,
                timestamp,
            )
            return False
        return True

plugwise_usb/nodes/circle.py (1)

632-632: Remove debug log statement

The debug statement "HOI restored_logs=%s" appears to be a temporary debugging aid and should be removed.

-                   _LOGGER.debug("HOI restored_logs=%s", restored_logs)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.

plugwise_usb/nodes/circle.py (4)

27-34: Use MAX_LOG_HOURS for recency window (aligns with configuration and EnergyCounters)
The PR objective targets handling logs outside MAX_LOG_HOURS. Using DAY_IN_HOURS hard-codes 24h and diverges from configured behavior and the pulses logs pruning (which uses MAX_LOG_HOURS). Align both to avoid inconsistencies.

Apply this diff:

 from ..constants import (
-    DAY_IN_HOURS,
+    DAY_IN_HOURS,
+    MAX_LOG_HOURS,
@@
-        A timestamp from within the last 24 hours is considered recent.
+        A timestamp newer than MAX_LOG_HOURS is considered recent.
@@
-        if age_seconds > DAY_IN_HOURS * 3600:
+        if age_seconds > MAX_LOG_HOURS * 3600:

Optional: consider downgrading the log level to info/debug to reduce noise during normal catch-up scenarios.

Also applies to: 583-589

---

467-474: Initial collection loop will terminate prematurely when cache is disabled (depends on fix above)
Because energy_log_update currently ties its return value to cache writes, this loop breaks on valid-but-uncached records. After decoupling any_record_stored from caching (see earlier comment), this loop will behave as intended.

---

550-557: Decouple return value from cache state to avoid false negatives when cache is disabled

any_record_stored is only set when _energy_log_record_update_state returns True, but that returns False when caching is disabled. This makes energy_log_update return False even when valid records were added to in-memory counters, breaking initial collection and rollover handling.

Apply this diff to set any_record_stored based on processing of recent, non-empty slots, regardless of cache writes, and avoid unnecessary save attempts when cache is disabled:

-            if await self._energy_log_record_update_state(
+            await self._energy_log_record_update_state(
                 response.log_address,
                 _slot,
                 log_timestamp.replace(tzinfo=UTC),
                 log_pulses,
                 import_only=True,
-            ):
-                any_record_stored = True
+            )
+            any_record_stored = True
             if not last_energy_timestamp_collected:
                 # Collect the timestamp of the most recent response
                 self._last_collected_energy_timestamp = log_timestamp.replace(
                     tzinfo=UTC
                 )
@@
-        if any_record_stored:
+        if any_record_stored and self._cache_enabled:
             _LOGGER.debug(
                 "Saving energy record update to cache for %s", self._mac_in_str
             )
             await self.save_cache()

Also applies to: 570-575

---

634-645: Prune loaded cache using MAX_LOG_HOURS instead of DAY_IN_HOURS
Keep the same recency window everywhere to prevent reloading/pruning inconsistencies versus EnergyCounters.get_pulse_logs().

-        skip_before = datetime.now(tz=UTC) - timedelta(hours=DAY_IN_HOURS)
+        skip_before = datetime.now(tz=UTC) - timedelta(hours=MAX_LOG_HOURS)

plugwise_usb/nodes/circle.py (2)

632-633: Remove stray debug log (“HOI restored_logs”)
This looks like a leftover dev log and is noisy.

-                    _LOGGER.debug("HOI restored_logs=%s", restored_logs)

---

541-547: Nit: fix typo in comment
Minor grammar nit in the inline comment.

-                # Don't store an old log-record, store am empty record instead
+                # Don't store an old log-record; store an empty record instead

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.